-
Notifications
You must be signed in to change notification settings - Fork 25
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[WIP] Implementation of absorbing boundary layer #83
base: master
Are you sure you want to change the base?
Conversation
Thanks @cjvogl for working on this. I'll test it out a bit more and it would be great if we can get this incorporated soon. I'm not sure why the travis tests failed, it seems to have been a server error on the apt-get update, so I'll try restarting the tests. They pass on my laptop. |
OK, the tests pass! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general! A few comments inline.
.gitignore
Outdated
@@ -3,7 +3,9 @@ xclaw | |||
*.mod | |||
*~ | |||
.data | |||
.data_abl |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think .data_abl or .output_abl are generated any longer with the new Makefile_abl
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch.
abldata.depth_upper[1] = 0.2 | ||
# Set additional parameters if needed | ||
# 1) trigonometric - no additional parameters needed | ||
# 2) appelo_colonius - epsilon, p, q (e.g. {'epsilon': 1e-4, 'p': 4, 'q': 4}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should perhaps add some info about these parameters to the README.md
file, or start a new abl.rst
file in $CLAW/doc/doc
?
# 3 or 'wall' => solid wall for systems where q(2) is normal | ||
# velocity | ||
|
||
clawdata.bc_lower[0] = 'extrap' # at xlower |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps comment somewhere that with ABL it's best to set extrap
BCs at the edges of the layer, which is what's done here.
Absorbing layer updates
…nto absorbing_layer
@cjvogl: looking at this some more, it is not clear to me why you need It would be nice to avoid extra aux arrays as much as possible, particularly for the 3D case and with AMR. Especially since these these arrays just have the value 1 everywhere except in the layers I guess? I also notice that in |
@rjleveque: once in the mapped coordinate frame, the system to be solved is Q_t + s(x)*AQ_x + s(y)*BQ_y + s(z)*CQ_z = 0. Thus, we either need to have the values of s(x_i), s(y_j), s(z_k) or maybe x_i, y_j, and z_k available to wherever we want to implement the layer (currently in As for the extra memory footprint, I agree that this approach sacrifices memory to minimize how invasive the ABL implementation is. @mandli and I briefly discussed some alternative approaches, where it seemed like reserving memory for these values only in the absorbing layer being the most efficient. I think we also agreed that @mjberger would need to be brought on-board to discuss the best way to do this. As for the multiply-by-one inefficiency, I can certain replace the related do loops with a where loop. I don't have enough experience to to speak to the efficiency of one approach (nested loops without logic) over the other (a "where" statement that involves logic) once the compiler optimizations happen though. If the "where loop" is the way to go, I'll get that changed. |
i don’t know what we’re talking about really, but can it be computed rather than stored? Math is cheap, memory bandwidth is slow.
… On Aug 20, 2018, at 2:26 PM, Chris Vogl ***@***.***> wrote:
@rjleveque <https://github.com/rjleveque>: once in the mapped coordinate frame, the system to be solved is (in 2d) Q_t + s(x)*AQ_x + s(y)*BQ_y + s(z)*CQ_z = 0. Thus, we either need to have the values of s(x_i), s(y_j), s(z_k) or maybe x_i, y_j, and z_k available to wherever we want to implement the layer (currently in flux2.f90). You are correct that for the majority of cells in the layer, all but one of these values is 1, and that it is only in the corners where all 3 will have values less than 1.
As for the extra memory footprint, I agree that this approach sacrifices memory to minimize how invasive the ABL implementation is. @mandli <https://github.com/mandli> and I briefly discussed some alternative approaches, where it seemed like reserving memory for these values only in the absorbing layer being the most efficient. I think we also agreed that @mjberger <https://github.com/mjberger> would need to be brought on-board to discuss the best way to do this.
As for the multiply-by-one inefficiency, I can certain replace the related do loops with a where loop. I don't have enough experience to to speak to the efficiency of one approach (nest loops without logic) over the other (a "where" statement that involves logic) once the compiler optimizations happen though. If the "where loop" is the way to go, I'll get that changed.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#83 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA1DC0oylLCLKP_7nmCFLs27UK2Ia0Qmks5uSymMgaJpZM4VdSCS>.
|
@mjberger I can give you a brief synopsis of the issue in the near future but I will try summarize. The basic problem is that we want to save the speeds coming out of the Riemann solvers so that the BCs can be set to slow down these speeds appropriately. @cjvogl has been using aux arrays for this which of course even in classic is using more memory than needed but for AMR this becomes a very large overhead penalty given we only need these at the true boundaries of the domain. The problem then becomes detecting if we are at a true domain boundary and having appropriately sized storage at the edges or redoing the calculations at the boundaries when necessary and having all the correct information required. |
@mjberger: yes, I apologize for the lack of details... a write-up of this is in progress. To expand on what @mandli said, we need to be able to compute an scaling factor that is applied to the wave info that comes out of the Riemann solver. In order to compute that scaling factor, we need to know where the grid cell edge is relative to the absorbing layer (that surrounds the domain). I dont recall there being an easy way to obtain grid cell location from flux2.f90... hopefully I’m mistaken.
… On Aug 21, 2018, at 9:01 AM, Kyle Mandli ***@***.***> wrote:
@mjberger I can give you a brief synopsis of the issue in the near future but I will try summarize. The basic problem is that we want to save the speeds coming out of the Riemann solvers so that the BCs can be set to slow down these speeds appropriately. @cjvogl has been using aux arrays for this which of course even in classic is using more memory than needed but for AMR this becomes a very large overhead penalty given we only need these at the true boundaries of the domain. The problem then becomes detecting if we are at a true domain boundary and having appropriately sized storage at the edges or redoing the calculations at the boundaries when necessary and having all the correct information required.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub, or mute the thread.
|
It would be nice if we knew the x-y coordinates of cells in Since we need to get v5.5.0 out, I suggest we not merge this just yet and have some more joint discussion of the best way to do this before proceeding. |
Not sure why the Travis checks failed, but I tried a refactor of the ABL implementation based on some discussions with @rjleveque. I tried for a good balance between multiplying-by-one in a large section of the for loops that needed to be modified and creating too many additional data structures... let me know what you all think. |
The following was in the Travis log:
It looks like |
Ah, that's right: |
Implemented absorbing boundary layer and modified acoustics_2d_radial example to add an ABL. Makefile_abl, setrun_abl.py, and setplot_abl.py are provided to make use of the new ABL routines.
This PR depends on clawpack/clawutil#129 and should not be merged until that PR is merged.